[AI-7115] Add a request retry strategy to the async GitHub client by AAraKKe · Pull Request #24963 · DataDog/integrations-core

AAraKKe · 2026-08-24T12:09:28Z

What does this PR do?

Adds a retry strategy to the async GitHub client for the failures that are not rate limiting, and separates it from the rate-limit handling that was already there.

Structure worth knowing before reading the diff:

Two layers, deliberately nested. _request (retries) wraps _rate_limited_request (today's loop, renamed). That order matters: each retry re-acquires the limiter, so it waits out any pause the governor is holding. Rate-limit responses stay owned by the inner layer and are never retried by the outer one.
retry.py describes, stamina executes. RetryPolicy is data: what to retry on, how many attempts, what backoff. No sleeping or backoff arithmetic of ours.
Defaults are chosen per endpoint by whether the request can be replayed, not by verb. Three mutating endpoints are idempotent and say so at their call site. Every method takes retry= to override, and policies compose.
A guard sits above any policy: auth failures, rate-limit responses, the limiter's give-up signal and redirects are never retried, however a caller configures things.
download_artifact retries as a pair. The signed URL expires, so the retry has to re-resolve the redirect rather than refetch a dead URL.
Config tunes the ladder only ([dispatcher.github_retries]). Widening what may be retried would make a duplicate side effect a setting.

ddev/src/ddev/utils/github_async/AGENTS.md documents the layer boundary so the next change lands in the right one.

Motivation

Closes AI-7115.

Dispatcher runs for hours and makes thousands of GitHub calls, and until now any failure that was not rate limiting failed on the first attempt. That gives a single blip more power than it should have. TaskTestRunner polls get_workflow_run for the whole life of a batch inside a try/finally with no except, so one transient 500 aborts the batch, closes its check run as cancelled and throws away the results of every test in it. Other calls swallow the failure and quietly degrade instead: a failed list_workflow_jobs returns an empty job list, so job correlation silently loses data.

Both get worse as we scale up: more batches and more polling mean more chances to hit the one blip that costs a whole batch of test results. Retrying is also a precondition for trusting the run report, since a report that is missing jobs because of a dropped connection is worse than one that is late.

No task behaviour changes here. Retries only make those paths less likely to fire, and a failure that outlives the ladder surfaces exactly as it does today.

Notes for review

"Retry" already means re-running failed test jobs in Dispatcher (ExecutionState.RETRYING, BatchProgress.retrying_jobs). This is unrelated and only concerns HTTP requests.
test_no_retry_on_transport_error became test_the_rate_limit_layer_does_not_retry_a_transport_error and now calls _rate_limited_request. The property still holds for that layer, but at client level a GET transport error is now retried on purpose.
The artifact policy adds 403 because that is how an expired signed URL presents from the storage host. GitHub's own 403 arrives as GitHubAuthenticationError, which the guard refuses, so a real denial still fails immediately. Tested both ways.
Open question, no action taken: stamina installs a process-wide hook that logs every scheduled retry to the stamina logger, so retries are visible even with no logger injected. Turning it off is global and would also silence the unrelated stamina.retry in ddev/e2e/agent/docker.py, so I left it and documented it. Say the word if you want the client to be the only voice.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Add qa/required if this PR needs QA validation, or qa/skip-qa if it does not. Exactly one of the two is required.
If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

- New retry.py: RetryPolicy plus composable predicates, executed by stamina. - Split the two layers: _request retries, _rate_limited_request handles rate limits. - Per-endpoint defaults by replay safety, overridable per call with retry=. - Never follow or retry an unexpected redirect; report it with the endpoint. - Retry the artifact redirect and signed download as a pair. - Expose the limits through [dispatcher.github_retries].

cit-pr-commenter-54b7da · 2026-08-24T12:11:34Z

evalya-impact-summary

evalya impact analysis

Impact analysis: 0 selected, 0 skipped (of 0 test tasks)
Publish tasks:   2 (always emitted)
Diff (14 files):
  ddev/changelog.d/24963.added
  ddev/src/ddev/cli/ci/tests/dispatcher_config.py
  ddev/src/ddev/utils/github_async/AGENTS.md
  ddev/src/ddev/utils/github_async/__init__.py
  ddev/src/ddev/utils/github_async/client.py
  ddev/src/ddev/utils/github_async/retry.py
  ddev/src/ddev/utils/github_errors.py
  ddev/tests/cli/ci/tests/test_dispatcher_config.py
  ddev/tests/utils/github_async/conftest.py
  ddev/tests/utils/github_async/helpers.py
  ddev/tests/utils/github_async/test_client_core.py
  ddev/tests/utils/github_async/test_download_artifact.py
  ddev/tests/utils/github_async/test_rate_limiting.py
  ddev/tests/utils/github_async/test_retry.py

Debug a specific task: evalya plan impact --path <path> --task <task>

Learn more about CI impact filtering

datadog-prod-us1-6 · 2026-08-24T12:12:23Z

✅ All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 88.92% (+0.12%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 78bdf90 | Docs | View more details | Give us feedback!}

AAraKKe · 2026-08-24T13:17:39Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c86bea94d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

- Move the retry config into dispatcher_config, next to the other config models. - Group module constants at the top of retry.py and trim the comments. - RetryPolicy is a plain class with a typed replace instead of a dataclass. - Move the client-specific guard and the retry cause into the client module. - Redact the query string from the artifact URL before logging it.

AAraKKe · 2026-08-24T15:38:57Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3b5071d14

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

AAraKKe · 2026-08-24T16:09:35Z

@codex review

httpx builds its message from the full URL, so a retryable failure from the storage host carried the presigned signature into this client's log line and stamina's retry hook. Raise without the URL instead of redacting at each sink.

chatgpt-codex-connector

💡 Codex Review

integrations-core/ddev/src/ddev/utils/github_async/client.py

Line 331 in f3b5071

raise type(exc)(f"{method} {endpoint}: {exc}") from exc

Preserve request context when wrapping transport errors

When any HTTPX transport failure occurs, the original exception has already been associated with the outgoing request, but constructing a replacement with only a message discards that context. Callers using the standard exc.request.url pattern will therefore get RuntimeError: The .request property has not been set, and error reporting loses the request metadata; pass the original request when constructing the replacement or preserve and re-raise the existing exception.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…logs A transport error's reason is quoted into our message and, with a chained cause, printed in full by Python. Both were URL-free only because of how httpx builds that message. Redact the query string and drop the chain instead.

Keeps the parameter names, which say which signing scheme was in play, and masks every value rather than the ones known to be secret: the parameter carrying the signature is X-Amz-Signature on S3 and sig on Azure Blob, so an allowlist would leak the first time a download redirects somewhere new.

Building a replacement exception dropped the request httpx had attached, so exc.request raised RuntimeError for a caller. Rewrite the message in place, as the artifact download already does.

AAraKKe · 2026-08-24T16:28:06Z

On the .request finding from the last review (client.py#L331, preserve request context when wrapping transport errors): valid, and fixed in 97f6e0e.

Worth noting it is not introduced by this PR. The same wrapping sits at client.py:226 on master, so exc.request has been raising RuntimeError for a caller since well before this branch. I ran into it in the new artifact path while looking at the signed URL leak and fixed it there by rewriting the message in place instead of constructing a copy, which keeps the request and the original frames.

Applied the same fix to _execute_request here rather than in a follow-up: it is one line, no test depended on the old behaviour, and leaving two different wrapping idioms in the same file would just be a trap for whoever reads it next. There is now a test asserting a transport failure still exposes the request it failed on, which reproduces the RuntimeError if the fix is reverted.

AAraKKe · 2026-08-24T16:30:33Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 97f6e0ef83

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The defaults are shared for the life of the process, so tuning one in place changed every client that held it. Frozen dataclasses with __post_init__ validation are the idiomatic way to prevent that; the alternatives for a plain class cost more boilerplate for the same result.

HadhemiDD

Thanks @AAraKKe ! This looks great, I just left some minor comments/questions

HadhemiDD · 2026-08-25T14:07:42Z


+def is_redirect_status(status_code: int) -> bool:
+    """Whether `status_code` is a redirect, Location header or not."""
+    return status_code in REDIRECT_STATUS_RANGE


304 status code falls under this range but it is a Not Modified, it has no Location.
We either set the status codes manually of add an exception for when it is a 304.

Good catch. Switched the guard to httpx's has_redirect_location (301/302/303/307/308 and a Location present), so a 304 no longer reports a redirect to a Location that does not exist.

The artifact endpoint still gets any 3xx handed back, since it validates the status and the Location itself and reports a bad one more precisely.

HadhemiDD · 2026-08-25T14:35:21Z

+def with_query_masked(text: str, url: str) -> str:
+    """`text` with the query of `url` masked, for a message someone else built out of that URL."""
+    query = url.partition("?")[2]
+    return text.replace(query, masked_query(query)) if query else text


Question: doesn't httpx re-encode the query? we match the query as a raw string under masked_query

It does for some values: a raw space comes back as %20. The old code matched the query against the URL we passed in, so that mismatch would have silently left the signature in the message.

Dropped the matching entirely. The whole query is now replaced with *** without parsing any of it, and a status error's reason is built from status_code/reason_phrase instead of rewriting httpx's message.

HadhemiDD · 2026-08-25T14:42:59Z

+    assert policy.should_retry(_status_error(502))
+
+
+def test_a_shared_default_cannot_be_retuned_in_place() -> None:


Suggestion: I would drop this test: it asserts dataclass(frozen=True) raises — CPython, not our code.

Agreed, dropped. Frozen also cannot regress unnoticed: a non-frozen RetryPolicy cannot be a field default, so the module stops importing. Replaced it with a test covering replace.

…hub-client-retry # Conflicts: # ddev/src/ddev/cli/ci/tests/dispatcher_config.py

- A 304 is no longer reported as a redirect. The guard is httpx's `has_redirect_location`, so only a Location the client declines to follow raises `GitHubUnexpectedRedirectError`; the artifact endpoint still gets any 3xx back to validate the status and Location itself. - A signed URL loses its whole query instead of having values masked parameter by parameter, and a status error's reason is built from the response rather than by rewriting httpx's message. Nothing about the query is parsed, so no encoding or delimiter has to be guessed right. - Dropped the test asserting a frozen dataclass refuses assignment, which is CPython's behaviour rather than ours, and kept one covering `replace`.

lucia-sb · 2026-08-26T12:25:06Z

+        retry_policies: RetryPolicies | None = None,
+        logger: logging.Logger | None = None,


Question: These new arguments aren’t set here. I assume that’s because the PR’s aim is to update the client rather than the dispatcher, but I wanted to mention it just in case since Claude flagged it as a request.

This is just so the client has a logger we can control, but right now everything that has to do with the logger is bound to change when we implement the monitoring part. This is all very temporary but yes, they would need to be injected.

Although I am still unsure if I will be handling it like that or through context vars... still unknown

lucia-sb · 2026-08-26T12:36:48Z

Suggestion: these tests exercise the retry layer they're meant to isolate from.

Before this PR, _request was the only request method — it owned rate-limit pacing and retries. This PR splits that in two: the old _request was renamed to _rate_limited_request, and a new _request was introduced above it that adds the non-rate-limit retry strategy (retry.py / stamina).

This file's job is to test the rate-limit layer in isolation, but only one test (test_the_rate_limit_layer_does_not_retry_a_transport_error, line 220) was updated to call the renamed _rate_limited_request. The other six — lines 116, 132, 163, 187, 203, and 235 — still call client._request(...), so they now run through the new outer retry guard (_refuses_retry) as well as the rate-limit logic they're supposed to be testing alone. In practice this mostly still passes today (the guard categorically refuses to retry rate-limit-confirmed responses, so the outcomes happen to line up), but it means a regression in either layer could now surface as a test failure in the other layer's file, which defeats the point of having this file separate from test_retry.py.

Suggested fix — repoint these six calls to _rate_limited_request, matching line 220:

- await client._request("GET", "/x") + await client._rate_limited_request("GET", "/x")

at lines 116, 132, 163, 187, 203, and 235. No other changes needed — the assertions and comments at each site still describe the right behavior once they're calling the right layer.

Thanks for the catch! renamed them so we are not using retries in there.

The file exists to test rate-limit pacing in isolation, but six calls still went through `_request`, which now adds the retry strategy on top. No behaviour changes today, since the guard refuses every rate-limit and auth failure these tests use. It matters because the file does not opt into `instant_backoff`, so anything that made the outer layer retry here would sleep on real backoff instead of failing. Also drops a duplicated assertion in the policy-tuning test.

dd-octo-sts · 2026-08-26T13:46:26Z

Validation Report

All 21 validations passed.

Show details

Validation	Description	Status
`agent-reqs`	Verify check versions match the Agent requirements file	✅
`ci`	Validate CI configuration and code coverage settings	✅
`codeowners`	Validate every integration has a CODEOWNERS entry	✅
`config`	Validate default configuration files against spec.yaml	✅
`dep`	Verify dependency pins are consistent and Agent-compatible	✅
`http`	Validate integrations use the HTTP wrapper correctly	✅
`imports`	Validate check imports do not use deprecated modules	✅
`integration-style`	Validate check code style conventions	✅
`jmx-metrics`	Validate JMX metrics definition files and config	✅
`labeler`	Validate PR labeler config matches integration directories	✅
`legacy-signature`	Validate no integration uses the legacy Agent check signature	✅
`license-headers`	Validate Python files have proper license headers	✅
`licenses`	Validate third-party license attribution list	✅
`metadata`	Validate metadata.csv metric definitions	✅
`models`	Validate configuration data models match spec.yaml	✅
`openmetrics`	Validate OpenMetrics integrations disable the metric limit	✅
`package`	Validate Python package metadata and naming	✅
`qa-label`	Validate the pull request declares whether it needs QA for the next Agent release	✅
`readmes`	Validate README files have required sections	✅
`saved-views`	Validate saved view JSON file structure and fields	✅
`version`	Validate version consistency between package and changelog	✅

View full run

AAraKKe added the qa/skip-qa Automatically skip this PR for the next QA label Aug 24, 2026

Add changelog entry

c86bea9

dd-octo-sts Bot added the ddev label Aug 24, 2026

chatgpt-codex-connector Bot reviewed Aug 24, 2026

View reviewed changes

Comment thread ddev/tests/utils/github_async/test_retry.py Outdated

chatgpt-codex-connector Bot reviewed Aug 24, 2026

View reviewed changes

Comment thread ddev/src/ddev/utils/github_async/client.py Outdated

Keep the signed artifact URL out of retry logs

2ab07c1

httpx builds its message from the full URL, so a retryable failure from the storage host carried the presigned signature into this client's log line and stamina's retry hook. Raise without the URL instead of redacting at each sink.

chatgpt-codex-connector Bot reviewed Aug 24, 2026

View reviewed changes

AAraKKe added 3 commits August 24, 2026 18:15

Keep the request attached when wrapping a transport error

97f6e0e

Building a replacement exception dropped the request httpx had attached, so exc.request raised RuntimeError for a caller. Rewrite the message in place, as the artifact download already does.

chatgpt-codex-connector Bot reviewed Aug 24, 2026

View reviewed changes

Comment thread ddev/src/ddev/utils/github_async/retry.py

AAraKKe marked this pull request as ready for review August 24, 2026 16:50

AAraKKe requested a review from a team as a code owner August 24, 2026 16:50

dd-octo-sts Bot added the team/agent-integrations label Aug 24, 2026

HadhemiDD requested changes Aug 25, 2026

View reviewed changes

AAraKKe added 2 commits August 26, 2026 11:05

Merge remote-tracking branch 'origin/master' into aarakke/AI-7115-git…

21d0e08

…hub-client-retry # Conflicts: # ddev/src/ddev/cli/ci/tests/dispatcher_config.py

lucia-sb reviewed Aug 26, 2026

View reviewed changes

lucia-sb approved these changes Aug 26, 2026

View reviewed changes

AAraKKe enabled auto-merge August 26, 2026 13:51

		assert policy.should_retry(_status_error(502))


		def test_a_shared_default_cannot_be_retuned_in_place() -> None:

		retry_policies: RetryPolicies \| None = None,
		logger: logging.Logger \| None = None,

Uh oh!

Conversation

AAraKKe commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Notes for review

Review checklist (to be filled by reviewers)

Uh oh!

cit-pr-commenter-54b7da Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

evalya-impact-summary

Uh oh!

datadog-prod-us1-6 Bot commented Aug 24, 2026 • edited by datadog-datadog-prod-us1 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AAraKKe commented Aug 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

AAraKKe commented Aug 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

AAraKKe commented Aug 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

AAraKKe commented Aug 24, 2026

Uh oh!

AAraKKe commented Aug 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

HadhemiDD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dd-octo-sts Bot commented Aug 26, 2026

Validation Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AAraKKe commented Aug 24, 2026 •

edited

Loading

cit-pr-commenter-54b7da Bot commented Aug 24, 2026 •

edited

Loading

datadog-prod-us1-6 Bot commented Aug 24, 2026 •

edited by datadog-datadog-prod-us1 Bot

Loading